Skip to content

PMM-15331 Gate health on SEP provisioning - #5768

Closed
yyyyyyyan wants to merge 19 commits into
PMM-15316from
PMM-15331
Closed

PMM-15331 Gate health on SEP provisioning#5768
yyyyyyyan wants to merge 19 commits into
PMM-15316from
PMM-15331

Conversation

@yyyyyyyan

@yyyyyyyan yyyyyyyan commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Ticket number: PMM-15331

Feature build: SUBMODULES-0not yet created; required before merge (see Not run here)

Summary

With PMM_ENABLE_SEP set, pmm-server no longer reports its container healthy until the current start's SEP provisioning run has finished. HEALTHCHECK tests /v1/server/readyz, which covers pmm-managed alone, and supervisord starts pmm-managed at priority 14 against sep-provision's 20 — so the container goes healthy well before grafana-sep publishes the Grafana token files, and a side-car released by depends_on: {condition: service_healthy} comes up with Grafana authentication permanently inert.

HEALTHCHECK now runs /opt/healthcheck.sh: today's probe verbatim, plus one condition gated on SEP actually being in use — a marker at /srv/.sep_provisioned proving this start's run finished. grafana-sep writes it from its existing EXIT trap (keyed on the script's own exit status, so no exit path can forget it); entrypoint.sh clears it before exec supervisord, so it can only describe the current start. /v1/server/readyz is untouched.

A per-run marker rather than a file test, because publish_token writes its two files through separate mktemp/mv pairs and files from a previous run survive a restart — a stale-but-non-empty pair would go green while a replacement is being minted.

Base branch is PMM-15316 (#5762), not main. Please merge #5755 and #5762 first. #5759 is a sibling, not a dependency.

"SEP is in use" is a three-term predicate

PMM_ENABLE_SEP alone is not it. PMM_HA_ENABLE and PMM_DISABLE_BUILTIN_POSTGRES override it outright — entrypoint.sh says so in a warning — so all three sites that care test the same thing:

Site Behaviour
healthcheck.sh requires the marker only when PMM_ENABLE_SEP is set and neither flag is
entrypoint.sh a marker it cannot clear is fatal under the same condition, and only there
grafana-sep MARK_RUN=0 on every path that skips provisioning, so the marker means this start provisioned

An external Grafana database — GF_DATABASE_URL or GF_DATABASE_HOST without either flag — is deliberately not exempt, and this is the asymmetry worth reviewing. sep-secrets treats the two groups differently, and correctly:

Condition sep-secrets SEP Container health
PMM_HA_ENABLE / PMM_DISABLE_BUILTIN_POSTGRES, whatever GF_DATABASE_* says removes all six files refuses to start unaffected — the gate does not apply
GF_DATABASE_URL / GF_DATABASE_HOST alone publishes all four password files starts, Grafana auth inert never healthy — no marker is ever written

The precedence is load-bearing: the documented external-PostgreSQL recipe sets PMM_DISABLE_BUILTIN_POSTGRES=1 and GF_DATABASE_HOST together (documentation/docs/reference/third-party/postgresql.md:192-213), and sep-secrets bails on the first pair before it looks at GF_DATABASE_*. Exempting on GF_DATABASE_* instead would let those deployments through while holding the one case that genuinely breaks SEP.

That verdict is reached by the marker simply never being written, not by the healthcheck testing GF_DATABASE_* — the gate does not need to know why provisioning did not happen.

Timing

--start-period 25s → 720s, covering grafana-sep's nominal READY_TIMEOUT=600 plus ≤80s of bounded token validation. HEALTHCHECK options are image metadata and cannot depend on an env var, so this applies to every container. A healthy server is not delayed — a probe that succeeds during the start period marks it healthy immediately. Only the unhealthy verdict for a server that has never been ready moves, ≈37s → ≈732s; one that came up and then broke still fails in ~12s. Config.Healthcheck.Test becomes ["CMD","/opt/healthcheck.sh"]; nothing in this repo asserts on that literal.

Tested

build/ has no unit tests and no lint job — there is no shellcheck/shfmt in any workflow or Makefile target, and no workflow triggers on build/** — and api-tests pulls perconalab/pmm-server:3-dev-latest and never sets PMM_ENABLE_SEP, so CI cannot exercise any of this. Evidence is therefore static gates plus harnesses driving the shipped scripts directly.

  • shellcheck -S style and bash -n clean on all three scripts; git ls-files -s confirms 100755 on healthcheck.sh, which matters because COPY preserves the mode and the image drops to USER 1000.
  • The shipped healthcheck.sh, 11 scenarios against a stub readyz, one per acceptance criterion it touches. PMM_ENABLE_SEP unset: 200 → healthy, 503 → unhealthy, marker ignored either way. Embedded SEP: no marker → unhealthy, marker → healthy. Both ignore flags, and either of them combined with GF_DATABASE_HOST: healthy with no marker present at all. GF_DATABASE_HOST alone and GF_DATABASE_URL alone: unhealthy. A failing readyz still dominates every combination. PMM_ENABLE_SEP=true behaves as 1; yes does not enable.
  • The shipped grafana-sep's exit-0 paths, 6 cases with a redirected secrets dir: no skip path marks the run — PMM_ENABLE_SEP unset, both ignore flags, and both GF_DATABASE_* forms all exit 0 with the marker absent and the token files removed. An unwritable secrets directory stays FATAL and unmarked.
  • The shipped entrypoint.sh marker block, 13 combinations over both marker shapes — a plain file, and a directory standing in for one rm -f cannot clear. Fatal for embedded SEP and for either GF_DATABASE_* form alone; clean start under both ignore flags and with PMM_ENABLE_SEP unset; the marker is always cleared when it is removable.
  • grafana-sep exit paths, 15 cases against a fake Grafana and a psql stub: both skip paths, all three HA/disable-builtin + GF_DATABASE_* overlaps, unwritable secrets dir, reuse with a valid token, ambiguous check, malformed material, rejected mint, published mint, and an errexit abort. Plus the Grafana-wait timeout at the real unshortened 600s.
  • Neither docker-compose.yml nor api-tests/docker-compose.yml overrides pmm-server's healthcheck, so the image metadata governs both; only ps declares its own.

Not run here

  • The Feature Build matrix the ticket requires — first boot, restart with a valid token, restart where Grafana rejects it, permanent failure, PMM_ENABLE_SEP unset, and PMM_ENABLE_SEP=1 with GF_DATABASE_URL set. The harnesses exercise neither a real Grafana, nor supervisord ordering, nor real boot timings. A Feature Build is required before merge.
  • Wall clock from start to healthy on a real first boot — the number that would make 720s measured rather than well-sized.
  • api-tests, which need a live server. /v1/server/readyz is untouched, so readyz_test.go is unaffected by inspection, not by a run.

Known limitations

  • A failed provisioning run holds the container unhealthy for the container's life. sep-provision is autorestart = false, so a transient Grafana failure that trips a FATAL path never retries — and this change makes that terminal for health rather than merely logged. The ticket scopes retrying out and prefers a loudly unhealthy server to a silently broken side-car, with the reason in /srv/logs/sep-provision.log, but nothing tracks adding a bounded retry.
  • api-tests.yml loses a fast failure: a server that never becomes ready now burns the full --wait-timeout=100 instead of failing at ≈37s. CI still fails, less specifically.
  • Two pre-existing residuals in grafana-sep are not bounded by 720s: wait_for_grafana counts sleep time rather than wall clock, and the psql calls have no timeout. An undersized budget is recoverable — one successful probe restores healthy and resets FailingStreak — so the risk is a transient window, not a stuck container. Bounding both is deferred work with no issue filed.
  • The marker certifies a complete publish via the run's exit status, not by re-reading the two files. Every path reaching the write ran publish_token under errexit, and an uncaught signal sets $? to 128+signum, so no current path defeats it. A future exit-0 path added without clearing MARK_RUN would; a [ -s ] check on both token files inside the trap would make it a code invariant instead of a consequence of errexit discipline. Not added here — it guards a hypothetical future edit rather than a reachable state.

.env.example and build/AGENTS.md changes are recorded on the ticket as accepted deltas, since its AC describes only healthcheck behaviour.

ademidoff and others added 14 commits August 11, 2026 17:16
Add an opt-in integration that lets SEP, running in a side container on a
shared Docker bridge network, use PMM's embedded PostgreSQL for its
persistence layer.

When PMM_ENABLE_SEP is set, the entrypoint appends marker-delimited blocks to
postgresql.conf (listen_addresses) and pg_hba.conf (one scram-sha-256 rule per
attached Docker subnet, scoped to the sep database and role), then provisions a
non-superuser sep role owning a dedicated sep database. Nothing is published on
the host, and the postgres, pmm-managed and grafana accounts remain unreachable
over the network. Unsetting the variable reverts the configuration on the next
start and leaves the role and database intact.

The postgres data directory, password file and binary directory are now
declared once in the entrypoint and passed to the helper scripts via a
subshell-scoped export, replacing the /usr/pgsql-14 literals that were
duplicated across them.
With PMM_ENABLE_SEP set, pmm-server now provisions the Grafana service
account SEP authenticates users against and publishes its token to
/srv/sep-secrets, one file per canonical SEP settings name. This replaces
the manual minting step SEP's feature-build harness performs today with
admin:admin basic auth from outside the container, which breaks as soon as
the Grafana admin password changes.

The account is created directly in Grafana's own PostgreSQL database over
the local socket, the same way postgres-sep provisions the sep role as the
local superuser: Grafana exposes no credential-free path to create a
service account, and pmm-server holds no Grafana admin credential once an
operator has changed the admin password.

The helper runs as a supervisord one-shot rather than from the entrypoint,
because Grafana is neither running nor migrated - on a first boot its
schema does not exist at all - until long after the entrypoint has exec'd
supervisord. The token is revalidated against Grafana on every start and
replaced only when Grafana rejects it, since SEP reads the secrets
directory once at its own start and re-minting unconditionally would
revoke the token a running SEP is still holding.

A freshly minted token that Grafana rejects is fatal and is never
published, so a future change to Grafana's schema or token encoding
surfaces as a failed sep-provision instead of silent SEP auth failure.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Stop a failed provisioning run from respawning without bound. startsecs = 0
is required for a one-shot, but it also means the program never enters
BACKOFF, which is the only state startretries bounds - so autorestart =
unexpected respawned every non-zero exit immediately and forever, measured
at ~10 runs per second. That turned the deliberate exit-1 safety net into a
hot loop of pbkdf2 and api_key writes against Grafana's database. A failed
run is meant to be read, not retried, so autorestart is now false.

Re-assert the service account's org role on the keep-existing-token path.
The validator endpoint answers 200 for any role, so an account demoted in
the Grafana UI kept authenticating while SEP's org-role lookups silently
degraded.

Make the secrets directory group-writable (0770). The image supports an
arbitrary runtime uid in group 0, and 0750 left such a uid unable to write
the mountpoint shipped in the image, which is also why no supervisord
program in this repo pins user = pmm. Matches the permissive mode the other
/srv directories already use.

Also bound the authentication call with --max-time, remove the temporary
file if publishing fails part-way, keep the disabled path inert when the
directory is not writable, and correct the documented re-mint semantics:
the token is replaced when the file is missing or empty too, not only when
Grafana rejects it.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Report, rather than swallow, a token file that cannot be removed when
PMM_ENABLE_SEP is unset. The disabled path must not fail a start over a
directory this uid cannot write, but a token left in place is still
readable by SEP, so silence there contradicted the documented contract.

Raise the Grafana readiness budget to 600s. A one-shot cannot bound its own
restarts under supervisord, so a run that gives up is not retried until the
container restarts; waiting longer is what absorbs a slow first boot, which
is the transient failure this is actually exposed to.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Puts SEP's two remaining deployment secrets through the secrets directory
PMM-15280 established, one file per canonical SEP settings name, so
neither travels through the SEP container's environment where docker
inspect and every process in its PID namespace can read them.

The lifecycles differ deliberately. SECRET_KEY is generated once and
persisted on /srv, following /srv/.postgres_password: re-minting it signs
out every SEP session and changes the inter-service token derived from it
by HMAC. The three *__DATABASE__PASSWORD files are rewritten on every
start so rotating PMM_SEP_POSTGRES_PASSWORD takes effect on the next
restart.

The helper runs from the entrypoint rather than under supervisord because
neither value needs a running service, which puts both on disk before
supervisord starts and keeps them clear of the Grafana readiness wait.
Unlike grafana-sep it does not bail on an external Grafana database:
postgres-sep still provisions the sep role there, so SEP still needs its
password files.

CELERY__BEAT_DBURI is not written. SEP-1797 landed, so the beat store
resolves from the SEP database settings and percent-encodes the password
itself; an explicit URI here would persist as an override.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Five defects found reviewing the helper against the permission and
re-mint contracts it documents.

The published files took the group of whichever uid pmm-server runs
as, not the group of the directory, so the documented group_add: ["0"]
requirement only held while that uid's primary group was 0 - which is
true of the stock image and false of the arbitrary-uid deployments the
entrypoint supports. The mountpoint is now setgid, which Docker
carries into a fresh named volume, so the files belong to group 0
whatever uid writes them.

The reuse check read an unreachable Grafana as a rejected token and
re-minted, revoking the token a running SEP holds - the outcome the
design exists to prevent. The probe now reports the status it got,
retries a call it could not make, and treats an inconclusive answer as
fatal rather than as a rejection.

install -d on an existing directory also chmods it, which a uid that
does not own the mountpoint cannot do; it exited 0 only because the
mode it asked for happened to match the image. Creation and the
writability check are now separate, so the fatal fires only when the
directory really cannot be used.

On the keep path a psql failure in the secondary role repair aborted
under errexit before the already-valid token was republished; the two
are now in dependency order. The Grafana-not-on-embedded-Postgres
guard now removes the token files like the disabled path does, rather
than leaving a token it can no longer revalidate.

Also renames the directory override to PMM_SEP_SECRETS_DIR, which no
longer collides with the name SEP uses for its own mount, and records
in .env.example that the settings names must be left unset rather than
empty on the SEP side or the mounted files go unused.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Narrowing the persisted key back to 0600 was unconditional and therefore
errexit-fatal. chmod needs ownership, not write permission, and the
arbitrary-uid path lets a later start run as a uid that does not own what
an earlier one persisted - so a key this start could read and publish
perfectly well stopped pmm-server from booting instead. Verified against
percona/pmm-server: a 0640 key owned by uid 1000, read as uid 1002,
exited 1 and published nothing. entrypoint.sh guards its own /srv chmod
the same way this now does, and postgres-migration only chmods a
credential file it just created.

Softening that alone would have opened a second hole, since a command
substitution failing in argument position does not trip errexit: an
unreadable key would have published an empty SECRET_KEY over a good one.
The value is now read into a variable and checked before publishing.

Also documents in .env.example what an operator otherwise finds out the
hard way: where the key is persisted, that whitespace around
PMM_SEP_POSTGRES_PASSWORD is kept by PostgreSQL but stripped by SEP, and
that with SEP enabled these failures stop PMM Server rather than
degrading SEP alone - which the secrets directory becoming an entrypoint
concern is what changed.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Cuts the review fixes' comment additions from 16 lines to 10: drops
the remove_token_files note, whose symmetry with the disabled branch
six lines above is visible; drops the aside about setups that mount
nothing at the secrets path; and folds three restatements of the
group-0 requirement into one.

What is left names the facts a reader would otherwise undo: that the
setgid bit is what makes the group inheritance work, that unreachable
and rejected must stay distinct, that install -d chmods a directory it
did not create, the errexit ordering on the keep path, and that the
empty case arm is intentional.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Drop PMM_SEP_POSTGRES_PASSWORD from the environment once the last
consumer has run. It otherwise reaches supervisord and every child,
and pmm-managed-init trace-logs each variable it is handed, name and
value included, so PMM_TRACE would print the SEP database password.
Mirrors the POSTGRES_PASSWORD unset earlier in the entrypoint.

Also correct the .env.example wording for the two fatal conditions:
say plainly that an unwritable /srv/sep-secrets stops PMM Server, and
record that neither check runs under PMM_HA_ENABLE or
PMM_DISABLE_BUILTIN_POSTGRES, where PMM_ENABLE_SEP is ignored outright.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Drops the header paragraph restating the revalidation rule the
grafana_auth_status docblock already carries, and the two lines
restating a SQL predicate and an idempotency guard visible in the
statements below them. Tightens the rest.

Also corrects the DELETE comment, which described only the arm that
avoids the name collision and not the one that removes every token on
the account.

47 comment lines against 187 of code, from 63. Still above the 0.15 of
postgres-sep, which the remaining blocks account for: this script
writes another product's schema and reproduces its token encoding, so
the index names, the psql interpolation rule, the api_key.role
behaviour and the reason for touching the database at all are facts no
code in this repo can show.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
publish() interleaved the write and the rename per file, so a write that
died part-way through the set - a full volume being the realistic cause -
left SEP some files rotated and the rest stale. Verified against the
previous revision: a failure on the third password file rotated two of
them and left the third behind.

Split it into stage() and commit_staged(). Every fallible step now runs
before the first rename, so the same failure leaves the previous complete
set in place and the script still exits nonzero. The temporary is recorded
for cleanup immediately after mktemp rather than after the write, since
errexit abandons the rest of stage() on a failed write and an unrecorded
temporary is one the EXIT trap cannot find.

This narrows rather than closes the gap: the set is still four renames, so
a failure between them leaves the earlier files updated. Closing that needs
a directory swap the mountpoint contract rules out.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
The sep-provision supervisord unit this branch adds starts unconditionally
and is a no-op when PMM_ENABLE_SEP is unset, so its stdout log always
exists under /srv/logs and always reaches the archive. TestDownloadLogs
compares the archive against an exact list, so it fails on every build
until the file is named there.

Caught by pmm3-api-tests #7005 on the feature build.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Signed-off-by: Yan Orestes <yan.orestes@percona.com>
The container's HEALTHCHECK moves from an inline curl to
/opt/healthcheck.sh, which runs the same readyz probe and, only when
PMM_ENABLE_SEP is set, additionally requires a marker proving this
start's provisioning run reached a successful conclusion.

grafana-sep writes /srv/.sep_provisioned from its EXIT trap, keyed on
its own exit status, so every deliberate exit 0 publishes it and every
FATAL path leaves none. entrypoint.sh removes the marker before
exec supervisord, so its presence can only describe the current start.

The one exception is the guard that skips provisioning on a non-embedded
Grafana database, where the marker decision mirrors sep-secrets' own
precedence. Under PMM_HA_ENABLE or PMM_DISABLE_BUILTIN_POSTGRES the
marker is written and the container still reports healthy, whatever
GF_DATABASE_* says: sep-secrets bails on those two first and removes
every file SEP has, so SEP refuses to start rather than running inert,
and the documented external-PostgreSQL recipe sets both. Under
GF_DATABASE_URL or GF_DATABASE_HOST alone the marker is withheld, since
sep-secrets still publishes SEP's four password files there and a
released side-car would come up with Grafana authentication inert.

--start-period widens from 25s to 720s to cover grafana-sep's nominal
600s Grafana wait plus its bounded token validation. This delays only
the unhealthy verdict for a server that has never been ready; a probe
that succeeds during the start period marks the container healthy
immediately, as it does today.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
@yyyyyyyan
yyyyyyyan requested a review from a team as a code owner August 12, 2026 22:46
@yyyyyyyan
yyyyyyyan requested review from 4nte and maxkondr and removed request for a team August 12, 2026 22:46
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.69%. Comparing base (d23ff97) to head (07111eb).

Additional details and impacted files
@@              Coverage Diff              @@
##           PMM-15316    #5768      +/-   ##
=============================================
+ Coverage      45.66%   45.69%   +0.02%     
=============================================
  Files            418      418              
  Lines          43407    43407              
=============================================
+ Hits           19823    19835      +12     
+ Misses         21603    21590      -13     
- Partials        1981     1982       +1     
Flag Coverage Δ
admin 34.93% <ø> (ø)
agent 51.56% <ø> (+0.13%) ⬆️
managed 45.22% <ø> (-0.01%) ⬇️
vmproxy 72.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yyyyyyyan

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Gates PMM Server container health on completion of SEP Grafana token provisioning.

Changes:

  • Adds a SEP-aware healthcheck with an extended startup grace period.
  • Creates and clears a per-start provisioning marker.
  • Documents the health-ordering contract.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
build/docker/server/healthcheck.sh Adds the readiness and SEP-marker probe.
build/docker/server/entrypoint.sh Clears stale markers before startup.
build/docker/server/Dockerfile.el9 Installs and configures the new healthcheck.
build/ansible/roles/grafana/files/grafana-sep Publishes the marker after successful provisioning.
build/AGENTS.md Lists the new healthcheck script.
.env.example Documents SEP health-based startup ordering.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread build/docker/server/entrypoint.sh Outdated
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 313c6c7d-d698-48c4-862e-fc627fe6030f

📥 Commits

Reviewing files that changed from the base of the PR and between 9e6ed3c and 9c39161.

📒 Files selected for processing (6)
  • .env.example
  • build/AGENTS.md
  • build/ansible/roles/grafana/files/grafana-sep
  • build/docker/server/Dockerfile.el9
  • build/docker/server/entrypoint.sh
  • build/docker/server/healthcheck.sh
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • percona/pmm-qa (manual)
  • percona/pmm (manual)

Walkthrough

SEP provisioning now records successful credential publication. The server image uses a dedicated health check that validates readiness and SEP provisioning state. HA and external PostgreSQL configurations retain their existing health behavior.

Changes

SEP health gating

Layer / File(s) Summary
Provisioning marker lifecycle
build/ansible/roles/grafana/files/grafana-sep, build/docker/server/entrypoint.sh
SEP provisioning writes /srv/.sep_provisioned only after success. Server startup clears the marker before provisioning and fails if cleanup cannot complete.
Health check integration
build/docker/server/healthcheck.sh, build/docker/server/Dockerfile.el9, .env.example, build/AGENTS.md
The image runs /opt/healthcheck.sh, which probes readiness and requires the SEP marker when SEP is enabled. The health check start period is 720 seconds. Documentation describes the behavior and references the script.

Sequence Diagram(s)

sequenceDiagram
  participant Docker
  participant healthcheck.sh
  participant PMMReadinessEndpoint
  participant SEPMarker
  Docker->>healthcheck.sh: run container health check
  healthcheck.sh->>PMMReadinessEndpoint: probe readiness
  healthcheck.sh->>SEPMarker: validate /srv/.sep_provisioned when SEP is enabled
  healthcheck.sh-->>Docker: return healthy or unhealthy status
Loading

Possibly related PRs

  • percona/pmm#5700: Extends the SEP provisioning flow with provisioning markers and health gating.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary change: gating health on SEP provisioning.
Description check ✅ Passed The description includes the required ticket and feature build fields, explains the change, and documents testing and pending validation.

Comment @coderabbitai help to get the list of available commands.

Comment-only change. Drops the restatements of what the code and the
names already say, keeping the four things a future editor cannot infer:
why exit code 2 is avoided, why $? is captured first, why the marker
clear lives in the entrypoint, and why the guard follows sep-secrets'
precedence rather than its own condition order.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
The fatal fired on PMM_ENABLE_SEP alone, so an unremovable marker took the
container down under PMM_HA_ENABLE or PMM_DISABLE_BUILTIN_POSTGRES - the two
configurations the lines just above declare PMM_ENABLE_SEP ignored in, and
where grafana-sep marks the run whatever provisioning did. A marker it cannot
remove there misleads nothing, and refusing to start is worse than the outage
the gate is meant not to cause.

An external Grafana database alone stays fatal: grafana-sep withholds the
marker for it, so a stale one would hold the container green for good.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
PMM_ENABLE_SEP is ignored outright under PMM_HA_ENABLE or
PMM_DISABLE_BUILTIN_POSTGRES, so the health gate now tests that whole
predicate rather than the bare flag. It used to test the flag alone and rely
on grafana-sep marking a run it had skipped to release a gate that should
never have applied - the criterion held, but only because the skip branch
happens to exit within seconds.

Both skip paths therefore stop marking the run, including the long-standing
one for PMM_ENABLE_SEP unset, which marked a start whose token files it had
just deleted. The marker now means what it says: this start provisioned.

An external Grafana database alone is deliberately not exempt. SEP does start
there, with its Grafana authentication inert, which is the case the gate
exists to catch.

.env.example gains the configuration it omitted - an external Grafana
database never reports healthy - and drops the claim that health is
unchanged under the two ignore flags, which was true only after
sep-provision had run. The 720s start period is now stated as applying to
every deployment.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
The two preceding commits put back eight of the twelve comment lines the trim
had removed, in changes that were not about comments. Removed again, below the
post-trim baseline.

The marker-clear comment needed no edit at all: "Fatal only when SEP is in
use" was inaccurate before the condition was narrowed and accurate after, so
narrowing the condition was the whole fix. The healthcheck's new block
restated its own condition and repeated a warning entrypoint.sh already
prints. The cleanup comment keeps only the $?-capture ordering, which is the
one part not inferable from the code; MARK_RUN says what it does at each site
that clears it.

.env.example keeps the case it was missing - an external Grafana database
never reports healthy - and drops the start-period explanation, which is
image behaviour rather than something an operator sets here.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Where READY_TIMEOUT is set rather than in the Dockerfile: the reader who
raises it is the one who needs to know the image's start-period follows it.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
@yyyyyyyan

Copy link
Copy Markdown
Collaborator Author

Closing this. The gate it adds is no longer reachable: with #5755 closed, SEP provisioning became synchronous, and a marker that says "this start's provisioning finished" has nothing left to report.

The design was right for what it was built against. grafana-sep was a supervisord one-shot that could not provision until Grafana had booted and migrated, so it finished long after the container was up and answering /v1/server/readyz — health could go green while the token files were still absent, and a side-car gated on condition: service_healthy would read an empty directory. Hence /srv/.sep_provisioned, written on success and cleared each start.

sep-secrets is not that. It runs from the entrypoint, before supervisord, and exits non-zero on every failure:

# Unconditional: the script owns its own gates, so the files it published are still
# removed on the start after PMM_ENABLE_SEP is cleared.
bash /opt/ansible/roles/sep/files/sep-secrets
# The last consumer has run, so drop the password before exec'ing supervisord: otherwise
# every child inherits it, and pmm-managed-init logs each variable it is handed - name and

https://github.com/percona/pmm/blob/fb359bf70cb73b8ecb31aa3e8df422a1356a356e/build/docker/server/entrypoint.sh#L237-L240

With set -o errexit on line 2, a failed provisioning aborts the entrypoint and the container never starts — so it never reaches a healthcheck. A successful one has published all four files before supervisord exists, and /v1/server/readyz cannot answer until pmm-managed is running under it. Health already implies the files are there, and depends_on: {pmm-server: {condition: service_healthy}} keeps working unchanged.

Two things here would actively regress if carried forward. The --start-period goes from 25s to 720s, sized on grafana-sep's 600-second Grafana readiness wait — nothing waits for Grafana now, so that would just give every unhealthy container a twelve-minute grace period. And the operator documentation it adds explains that an external Grafana database means the container never reports healthy, because PMM could not provision a service account in a database it does not own; PMM no longer provisions one anywhere.

Stripping both leaves the marker protocol, and the marker is the part that became unnecessary. PMM-15331 resolves as no change needed rather than as work to reshape.

@yyyyyyyan yyyyyyyan closed this Aug 20, 2026
@marcuscruz-percona

Copy link
Copy Markdown

Came to review this as one of the four on the request, and it was already closed by the time I got here — so this is a note on the closure rather than a review of the diff.

Verdict

Agree with closing as no change needed. I checked the reasoning rather than taking it on trust, and it holds.

sep-secrets runs from the entrypoint, before supervisord, with set -o errexit on line 2 of that file — so a failed provisioning aborts the entrypoint and the container never reaches a healthcheck at all, and a successful one has published every file before supervisord exists. Since /v1/server/readyz cannot answer until pmm-managed is running under supervisord, health already implies the files are there, and depends_on: {pmm-server: {condition: service_healthy}} keeps working with nothing added. A marker saying "this start's provisioning finished" genuinely has no one left to tell. That is a different shape from the one this branch was built against, where a supervisord one-shot could not provision until Grafana had booted and migrated and so finished long after health went green.

The two carry-forward hazards named in the closing comment are the right ones to have caught. The 25s-to-720s --start-period was sized on a 600-second Grafana readiness wait that no longer exists, and would have handed every unhealthy container a twelve-minute grace period — and I see #5808 is sizing that question on its own terms instead, which is where it belongs. The operator documentation explaining that an external Grafana database means the container never reports healthy would have described behaviour that no longer exists once nothing provisions a service account.

One thing worth carrying forward, since it lands on the two open PRs rather than here. The conclusion "a failure aborts the entrypoint and the container never starts" is now the only signal for SEP provisioning problems, and it is PMM-wide fallout from a SEP-only flag. #5762 adds entrypoint-fatal paths for an unwritable secrets directory and an unreadable persisted key; #5759 adds one for a resolv.conf with no usable nameserver. Each is individually defensible, and together they mean a handful of SEP-specific misconfigurations now stop PMM Server from booting rather than degrading SEP alone. Worth one deliberate decision about whether that posture is what you want, in whichever of the two lands second — I have raised it on #5759 as well.

Good call reading the design against what it now sits on rather than reshaping it to fit. Resolving the ticket as no change needed is the honest outcome.

yyyyyyyan added a commit that referenced this pull request Aug 21, 2026
#5755 (PMM-15280, Grafana service account) and #5768 (PMM-15331, the health
gate that depended on it) were closed unmerged: provisioning the account by
writing Grafana's rows directly was the wrong shape, and with that gone SEP
provisioning is synchronous, leaving the gate nothing to report. The previous
derivation still carried both, so a paired bring-up exercised code that will
never ship - which is what the SEP side hit.

This derivation is main (now carrying PMM-15238) plus #5762, #5759, #5653,
#5739 and #5758. Recorded with -s ours so the branch moves forward without a
force-push; the tree is the re-derivation.

Signed-off-by: Yan Orestes <yan.orestes@percona.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants